Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Marking Cargo.lock as generated #6548

Merged
merged 1 commit into from
Jan 22, 2019

Conversation

dwijnand
Copy link
Member

12 weeks have passed, let's mark lock files as generated!

Fixes #6180

@rust-highfive
Copy link

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

Thanks!

I wonder if we could perhaps expand the comment a bit? Something along the lines of adding human-readable text about how it's a generated file and should not be edited, followed by @generated perhaps? I think it'd also be good to have a comment in the Cargo source indicating what @generated means and why we're emitting it.

@dwijnand
Copy link
Member Author

Sure! I was a little unsure, so I pushed my attempt. WDYT? I'm happy to keep tweaking it.

@dwijnand
Copy link
Member Author

I used https://github.com/github/linguist/blob/f1f4018d7a9950c1d9cf3d0887d994cf496d63c4/lib/linguist/generated.rb as inspiration (note Cargo.lock is already ignored there). Specifically, I preferred "Not intended for manual editing" over "Do not edit" (or "DO NOT EDIT").

@alexcrichton alexcrichton added the T-cargo Team: Cargo label Jan 15, 2019
@alexcrichton
Copy link
Member

This is going to affect every single Rust project and likely surprise a lot of people. I think we're ready for this change though and the surprise is mostly going to be a result of change, not that it's a change that shouldn't have happened. Regardless I'd like to make sure we're all on board with this, so..

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Jan 15, 2019

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@ehuss
Copy link
Contributor

ehuss commented Jan 15, 2019

This is going to affect every single Rust project

Just to be clear, the only effect is that if you check in the Cargo.lock into VCS, it will show up as dirty after the first build with a new cargo? If you don't check the lock file in, you probably won't even notice?

@alexcrichton
Copy link
Member

@ehuss an excellent clarification, and spot on

// https://github.com/rust-lang/cargo/issues/6180
// At the start of the file we notify the reader that the file is generated.
// Specifically Phabricator ignores files containing "@generated", so we use that.
let marker_line = "# This file is automatically @generated. Not intended for manual editing.";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excuse me, why there is two spaces here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rfcbot
Copy link
Collaborator

rfcbot commented Jan 18, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

psst @alexcrichton, I wasn't able to add the final-comment-period label, please do so.

@dwijnand dwijnand added the final-comment-period FCP — a period for last comments before action is taken label Jan 18, 2019
@lukaslueg
Copy link
Contributor

Maybe nitpicking but could we have a full English sentence instead of "Not intended for manual editing" here? E.g. "This file is not intended for manual editing" or "It is not ...". Also, if this is an auto-generated egg, where is the chicken - who generated it?

This file was automatically @generated by Cargo. It is not intended for manual editing.

@alexcrichton
Copy link
Member

@dwijnand want to follow up with @lukaslueg's thoughts and we can merge?

@dwijnand
Copy link
Member Author

I thought it was fine, but I don't mind tweaking it. I dropped the "automatically" because I want to keep it one line, under 80 chars, as looking in https://github.com/github/linguist/blob/f1f4018d7a9950c1d9cf3d0887d994cf496d63c4/lib/linguist/generated.rb it's probably best, over multi-line markers.

@alexcrichton
Copy link
Member

Hm I'm not sure I understand the restriction of one line, is that something that tools conventionally try to accept?

@dwijnand
Copy link
Member Author

Yes, it would appear so, from that linguist file.

But I realised we can just call the first one the marker line. So it's now two lines:

# This file is automatically @generated by Cargo.
# It is not intended for manual editing.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jan 22, 2019

📌 Commit bd0e4a0 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Jan 22, 2019

⌛ Testing commit bd0e4a0 with merge 3de188f...

bors added a commit that referenced this pull request Jan 22, 2019
…hton

Marking Cargo.lock as generated

12 weeks have passed, let's mark lock files as generated!

Fixes #6180
@bors
Copy link
Contributor

bors commented Jan 22, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: alexcrichton
Pushing 3de188f to master...

@bors bors merged commit bd0e4a0 into rust-lang:master Jan 22, 2019
@dwijnand dwijnand deleted the mark-lockfile-as-generated branch January 22, 2019 18:42
est31 added a commit to est31/cargo-local-serve that referenced this pull request Apr 12, 2019
est31 added a commit to RustAudio/deepspeech-rs that referenced this pull request Apr 24, 2019
est31 added a commit to est31/mimas that referenced this pull request Sep 3, 2020
@ehuss ehuss added this to the 1.34.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period FCP — a period for last comments before action is taken T-cargo Team: Cargo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Marking Cargo.lock as generated
7 participants